Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: event_payload column can be JSONB or TEXT #5372

Merged
merged 25 commits into from
Jan 9, 2025

Conversation

Sidddddarth
Copy link
Member

@Sidddddarth Sidddddarth commented Dec 13, 2024

Description

event_payload column in newly created jobs tables will be TEXT columns.

Allows migrations from one type to the other, so changing from one to the other shouldn't fail. Changing back to JSONB would allow server processes to continue, however utility postgres functions like unionjobsdb would fail in case of bytea column.

Not a reloadable selection.

Allowed creating BYTEA columns but only for benchmark purposes. TEXT seems to be doing better and also avoids other character validation problems.

Linear Ticket

Resolves PIPE-1649

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@Sidddddarth Sidddddarth marked this pull request as draft December 13, 2024 10:33
@Sidddddarth Sidddddarth force-pushed the chore.jobsdbPayloadColumnType branch from c053da4 to c5c21e3 Compare December 13, 2024 12:02
Copy link

codecov bot commented Dec 18, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 29 lines in your changes missing coverage. Please review.

Project coverage is 74.76%. Comparing base (4be15cb) to head (41f8474).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
jobsdb/migration.go 61.42% 22 Missing and 5 partials ⚠️
jobsdb/jobsdb.go 88.23% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5372      +/-   ##
==========================================
- Coverage   74.78%   74.76%   -0.02%     
==========================================
  Files         440      440              
  Lines       61538    61607      +69     
==========================================
+ Hits        46022    46062      +40     
- Misses      12979    13001      +22     
- Partials     2537     2544       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Sidddddarth Sidddddarth changed the title chore: event_payload column can be JSONB, BYTEA or TEXT chore: event_payload column can be JSONB or TEXT Dec 23, 2024
@Sidddddarth Sidddddarth marked this pull request as ready for review December 24, 2024 06:22
jobsdb/jobsdb.go Outdated Show resolved Hide resolved
jobsdb/jobsdb.go Outdated Show resolved Hide resolved
jobsdb/jobsdb.go Outdated Show resolved Hide resolved
@Sidddddarth Sidddddarth force-pushed the chore.jobsdbPayloadColumnType branch from f964bf8 to 5a4adff Compare December 26, 2024 12:29
jobsdb/integration_test.go Outdated Show resolved Hide resolved
jobsdb/jobsdb.go Outdated Show resolved Hide resolved
jobsdb/jobsdb.go Outdated Show resolved Hide resolved
jobsdb/jobsdb.go Outdated Show resolved Hide resolved
jobsdb/migration.go Outdated Show resolved Hide resolved
jobsdb/migration.go Outdated Show resolved Hide resolved
jobsdb/migration.go Outdated Show resolved Hide resolved
@@ -459,18 +459,85 @@ func (jd *Handle) getMigrationList(dsList []dataSetT) (migrateFrom []dsWithPendi
return
}

func getColumnConversion(srcType, destType string) string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to find a universal way to do the mapping, that will always work?

@Sidddddarth Sidddddarth force-pushed the chore.jobsdbPayloadColumnType branch from e5e9442 to b55615a Compare December 28, 2024 18:03
@cisse21 cisse21 force-pushed the chore.jobsdbPayloadColumnType branch from 0ee4339 to 1762246 Compare January 7, 2025 10:10
@cisse21 cisse21 requested review from ktgowtham and lvrach January 7, 2025 10:12
@cisse21 cisse21 force-pushed the chore.jobsdbPayloadColumnType branch from 1762246 to 3bc24b4 Compare January 7, 2025 10:19
@cisse21 cisse21 force-pushed the chore.jobsdbPayloadColumnType branch from 7808f82 to 935f63c Compare January 7, 2025 14:02
@cisse21 cisse21 requested a review from ktgowtham January 9, 2025 07:44
Copy link
Contributor

@ktgowtham ktgowtham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@cisse21 cisse21 merged commit b282c33 into master Jan 9, 2025
58 checks passed
@cisse21 cisse21 deleted the chore.jobsdbPayloadColumnType branch January 9, 2025 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants